fix(auth): keep the OAuth listTools request alive across every phase - #241
fix(auth): keep the OAuth listTools request alive across every phase#241umutkeltek wants to merge 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab62f071c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| preParsedRequest | ||
| ); | ||
| socket.write(JSON.stringify(response), () => { | ||
| const stopProgress = startProgressFrames(socket, preParsedRequest?.id ?? 'unknown'); |
There was a problem hiding this comment.
When a keep-alive server accepts resources/list or resources/read but never responds, this unconditional heartbeat continues while the daemon event loop remains healthy, and the client resets its 30-second socket deadline on every frame. Those runtime methods have no operation timeout, so the CLI now waits forever instead of timing out, restarting the daemon, and retrying as it did before this commit. Restrict these heartbeats to the bounded listTools path or retain an absolute recovery deadline for the other methods.
Useful? React with 👍 / 👎.
| // deadline, so a request stays alive for as many phases as it needs -- an OAuth | ||
| // code wait plus any number of paginated `tools/list` pages -- without the | ||
| // client having to predict how many phases there will be. | ||
| export const DAEMON_PROGRESS_INTERVAL_MS = 250; |
There was a problem hiding this comment.
When an API caller supplies ListToolsOptions.timeoutMs <= 250 (also accepted by --oauth-timeout), the client arms that same socket timeout immediately, but the host waits 250 ms before its first progress frame. Because the daemon starts the operation timeout only after receiving and dispatching the request, the socket deadline can fire first, causing invoke() to classify this as a transport failure and restart/replay the request instead of returning the non-retryable operation_timeout. Send an initial frame immediately or coordinate the interval with the requested timeout.
Useful? React with 👍 / 👎.
ab62f07 to
9a296ab
Compare
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs maintainer review before merge. Reviewed July 31, 2026, 12:07 PM ET / 16:07 UTC. ClawSweeper reviewWhat this changesThis PR forwards the OAuth timeout through tool discovery, keeps long OAuth-plus-pagination daemon requests alive with progress frames, and safely drains shared daemons during protocol replacement. Merge readinessKeep this PR open for maintainer review. The supplied PR evidence is strong and the prior concrete review findings appear addressed, but repository inspection could not run: every read-only command failed before execution with Priority: P1 Review scores
Verification
How this fits together
flowchart LR
CLI[CLI auth command] --> Client[Keep-alive daemon client]
Client --> Socket[Daemon socket protocol]
Socket --> Host[Daemon host]
Host --> Runtime[MCP runtime and OAuth]
Runtime --> Server[OAuth MCP server]
Host --> Result[Tool list or timeout]
Result --> CLI
Decision needed
Why: The branch deliberately chooses availability of the already-running peer request over completion of the upgrading caller. That compatibility and user-experience trade-off cannot be resolved safely from automated review evidence alone. Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve active shared-daemon requests during upgrade, but obtain maintainer agreement that a new client may fail loudly rather than preempt a busy legacy daemon; then re-run a full source and history review against the exact PR head. Do we have a high-confidence way to reproduce the issue? No independent high-confidence reproduction was possible in this review because all local read-only commands failed before execution. The contributor supplies a detailed real OAuth-and-pagination before/after run and targeted tests, but those claims could not be checked against the exact head here. Is this the best way to solve the issue? Unclear. The supplied branch directly addresses the reported replay and includes a non-preemptive upgrade safeguard, but maintainers must decide whether its bounded failure for a busy legacy daemon is the intended compatibility policy. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 7259c8c2c9b0. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (14 earlier review cycles; latest 8 shown)
|
`mcporter auth` runs the interactive OAuth browser flow inside the SDK's
`tools/list` request, but `listTools` never forwarded a per-request timeout. The
SDK's 60s DEFAULT_REQUEST_TIMEOUT_MSEC killed the request long before the 300s
OAuth code wait could finish, so slow providers could never be authorized --
every `mcporter auth` died with `MCP error -32001: Request timed out`.
Mirror the existing `callTool` timeout forwarding:
- add `timeoutMs` to `ListToolsOptions` and pass `{ timeout,
resetTimeoutOnProgress, maxTotalTimeout }` to `client.listTools`, with
`raceWithTimeout` as an outer guard;
- have the `auth` path pass `MCPORTER_OAUTH_TIMEOUT_MS` (default 300s) so the
request lives at least as long as the OAuth code wait;
- carry `timeoutMs` across the daemon protocol so keep-alive servers get the
same deadline, and report a phase that blows it as `operation_timeout` so the
keep-alive wrapper stops treating it as a dead server worth restarting;
- version the daemon protocol so a daemon started before this change is
replaced instead of silently dropping the new field.
9a296ab to
6d72274
Compare
|
Thanks — both review passes found real things. All three items are addressed and the PR body now carries an inspectable before/after run. Summary: P1 — liveness frames outliving a wedged request. The stated premise doesn't hold:
P2 — short deadlines expiring before the first frame. Fixed, and a step further than suggested: an immediate first frame alone still loses a sub-250ms deadline on the second gap, so the cadence is now derived from the caller's own deadline (sent on the request envelope) and the first frame is written immediately on dispatch. P3 — release-owned changelog. Correct, and I checked it against this repo's history rather than taking it on trust: my own merged #234 didn't touch Real behavior proof is in the PR body: a local OAuth-protected MCP server (dynamic registration, PKCE Each item has regression coverage; |
clawsweeper
Bot
added
proof: sufficient
6d72274 to
2d6d820
Compare
|
Fixed the remaining P2 — good catch, the finding is exact.
I did not reject short values at the input boundary instead: Regression added below the old floor, asserting the invariant directly rather than racing a timer:
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
Sizing the daemon socket deadline for a fixed number of phases cannot work. `McpRuntime.listTools()` issues one `tools/list` request per cursor page and gives each its own `timeoutMs`, so an OAuth wait followed by two or more slow pages outlives any constant multiple of the caller deadline. The socket then expires mid-flight, the client restarts the daemon and resends the listing, and the user is walked through a second OAuth flow. Stop predicting the phase count and observe the daemon instead. While a request is in flight the host emits newline-delimited progress frames, and the client treats each frame as proof of life and restarts its socket deadline. The deadline becomes a liveness budget rather than an operation budget: a request survives however many phases it needs, while a daemon that goes silent is still torn down and restarted exactly as before. Liveness alone would be too weak, so every operation stays provably bounded: - the first frame goes out immediately and the cadence is derived from the caller's own deadline, so a deadline shorter than the default interval cannot expire before the daemon has proved it is alive; - operations with a fixed phase count -- at most one connect plus one MCP request -- carry an absolute ceiling equal to the sum of the deadlines the daemon already applies to those phases, so a server that accepts a request and never answers yields a non-retryable `operation_timeout` rather than an indefinite wait; - `listTools` keeps its per-page deadline and gains a repeated-cursor guard, so the one operation whose phase count is data-dependent cannot page forever. Both peers decode frames incrementally, so the daemon's own status probe and stop handshake stay readable, and a response with no trailing newline still parses. Regression coverage drives the real client transport and the real host framing over a socket: an OAuth wait followed by three delayed `tools/list` pages resolves with one `listTools` request, no replay, and no daemon relaunch. Sibling cases cover a deadline below the default frame interval, a daemon that goes silent, and a server that repeats a pagination cursor.
2d6d820 to
0e37f7e
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
The previous code's protocol-version check classified any pre-v2 daemon as stale and called stop() on it from ensureDaemon(). A second client upgrading mid-request -- OAuth code wait or a delayed cursor page -- would race ahead of the first client's in-flight call, kill the daemon, and force the very replay the liveness mechanism was added to prevent. The fix makes the replacement an idle-drain transition, not a preempt: * host: the status response now reports the live in-flight count, sampled at response time so a long-running request still shows up as in flight. * protocol: StatusResult.activeRequests is optional so a v1 daemon that cannot advertise it does not break old clients. * client: before restartDaemon() issues stop(), it polls the live daemon up to MCPORTER_DAEMON_DRAIN_TIMEOUT_MS (60s default) for activeRequests to read zero. Pre-v2 daemons are treated as permanently busy, so the drain timeout is the only knob for them too. If the daemon is still busy past the timeout, the replacement is abandoned and the caller fails loud rather than killing the peer. Drain timeout is env-var overridable so the test suite can exercise the refusal branch without waiting the full minute.
Regression coverage for the Codex/ClawSweeper review round: * daemon-client-config-stale: two new tests pin the drain behavior. The first asserts a busy shared daemon is left alone until its in-flight count flips to zero, then replaced; the second asserts a daemon that never drains is left running rather than killed mid-request. * daemon-host: three new tests pin the operation ceiling for callTool, listResources, and readResource against a wedged runtime, using MCPORTER_OAUTH_TIMEOUT_MS=50 and fake timers so the OAuth-timeout-plus-60s-DEFAULT_REQUEST_TIMEOUT_MSEC ceiling fires in milliseconds rather than the production minute-plus. * daemon-listtools-progress: one new test exercises a 30ms caller deadline -- well under the 250ms default progress cadence -- to prove the first progress frame beats the deadline. The existing 120ms test already exercised this implicitly; the 30ms case makes the P2 fix visible in the test name.
clawsweeper
Bot
added
status: 👀 ready for maintainer look
The earlier drain landed a contract bug: it only checked whether the original daemon had gone idle, then immediately fell through to stop(). A second client that won the race to replace the busy daemon during the drain would see its fresh daemon killed by the upgrading client the moment the drain returned. Three changes close the gap: * client: add `probeLiveStatus` that sends a raw status probe without the pid-match filter `readVerifiedStatus` applies. `readVerifiedStatus` collapses pid mismatches into null, which would short-circuit the drain check and re-introduce the very regression the drain exists to prevent. * client: `waitForDaemonIdle` now returns both `drained` and `replacedByPeer`. The pid is tracked across polls so a swap mid-wait is visible at the moment the drain resolves. * client: `restartDaemon` uses `probeLiveStatus` and bails out on `replacedByPeer` instead of issuing `stop`. A third regression test pins the new path: a busy daemon that another client replaces mid-drain must not receive stop, and the upgrading client must not relaunch a daemon of its own.
The drain handles the case where a peer swaps in a fresh daemon *while* the upgrading client is waiting. A subtler race happens *before* the drain even starts: a peer has already replaced the busy daemon, but the on-disk metadata still names the old PID, so the previous early-return check (which also required a fresh config) did not fire and the code fell through to stop() against the peer's fresh daemon. Make the pid-mismatch short-circuit in restartDaemon unconditional: if the live daemon's pid does not match the expected one, do not stop it, whether or not the metadata still reads stale. The peer owns the replacement; the upgrading client just uses whatever daemon is live. This subsumes the pid-mismatch + fresh-config fast path, which is removed rather than stacked on. The transport-error path is unaffected because expectedPid is undefined there and the guard is skipped, so the client still self-heals when no daemon is responding. A new regression test pins the scenario: a peer wins the swap before the drain starts, the upgrading client must not call stop, and the peer's daemon must keep serving.
1 participant
Resubmission of #237 with the correctness gap fixed and a clean history, now updated for the Codex and ClawSweeper review round.
The gap in #237
resolveOperationSocketBudgetsized thelistToolssocket at2 * timeoutMs + 5s: one phase for OAuth, one for a singletools/list. ButMcpRuntime.listTools()walksnextCursorand gives every page its owntimeoutMs, so an OAuth wait plus two or more slow pages outlives the socket. The client sees a transport timeout, restarts the daemon, resends the listing, and the user gets the second OAuth flow this PR exists to prevent.Any constant multiple has the same defect — it encodes a phase count the runtime does not promise.
The fix: observe the daemon instead of predicting it
While a request is in flight the daemon host emits newline-delimited progress frames. The client treats each frame as proof of life and restarts its socket deadline. That turns the socket deadline from an operation budget into a liveness budget:
operation_timeout, which the client does not retry.resolveOperationSocketBudgetand its 5s grace are gone. ThelistToolssocket uses the caller deadline verbatim. Both peers decode frames incrementally (DaemonFrameDecoder), so the daemon's own status probe and stop handshake stay readable and a response with no trailing newline still parses. The daemon protocol is versioned, so a daemon started before this change is replaced rather than left speaking the old wire format.Real behavior proof
A local OAuth-protected MCP server (dynamic client registration, PKCE
S256, authorization code, bearer-gated/mcp) that paginatestools/listacross 4 pages at 4s each.mcporter authruns the real interactive browser flow — the browser launch is shimmed so approval lands 4s after the URL opens — routed through the real keep-alive daemon (lifecycle.mode = keep-alive, confirmed daemon pid).MCPORTER_OAUTH_TIMEOUT_MS=6000, so every individual phase is comfortably inside its deadline and only the total exceeds a fixed budget.Before — 462865b, the head that was closed (
2 * 6000 + 5000= 17s socket budget):After — this branch:
One authorization, one completed listing, no replay, no daemon restart. The replay in the "before" run begins 17.3s after the listing started, which is exactly the
2 * timeoutMs + 5sbudget — the failure mode described in the #237 review, reproduced and then removed.(The provider is a local fixture rather than a third-party service so the run is inspectable and repeatable; it exercises the real SDK OAuth client path — discovery, registration, PKCE, code exchange, bearer-gated MCP — not a stub.)
Review round: what changed
[P1] Liveness frames could outlive a wedged request. The premise as stated — that
listResources/readResourcehave no operation timeout — does not hold: every SDK request carriesDEFAULT_REQUEST_TIMEOUT_MSEC(60s) and every OAuth wait carriesDEFAULT_OAUTH_CODE_TIMEOUT_MS(300s). But the underlying worry was real and I found a stronger case than the one reported:McpRuntime.listTools()walksnextCursorwith no guard, so a server that repeats a cursor pages forever — and heartbeats would then keep the client waiting forever. Both are now closed:operation_timeoutinstead of an indefinite wait.listTools, whose page count is data-dependent, keeps its per-page deadline and gains a repeated-cursor guard in the runtime.[P2] Short deadlines could expire before the first frame. Fixed, and slightly further than suggested: the first frame is written immediately when the request is dispatched, and the cadence is derived from the caller's own deadline (
resolveProgressInterval, sent on the request envelope) rather than fixed at 250ms. An immediate frame alone would still lose a sub-250ms deadline on the second gap.[P3] Release-owned changelog. Correct — checked against this repo's history: my own merged #234 did not touch
CHANGELOG.md; the maintainer added the entry at release with thethanks @…credit line. The entry has been removed from this branch.[P1, ClawSweeper] An upgraded client could stop a live shared daemon mid-request. The protocol-version check classified any pre-v2 daemon as stale and called
stop()on it fromensureDaemon(). A second client upgrading mid-request — OAuth code wait or a delayed cursor page — could race ahead of the first client's in-flight call, kill the daemon, and force the very replay this PR is meant to remove. The replacement is now an idle-drain transition, not a preempt:statusresponse reports the live in-flight count (StatusResult.activeRequests, sampled at response time so a long-running request still shows as in flight). The field is optional, so v1 daemons that cannot advertise it still answer old clients.restartDaemon()polls the live daemon up toMCPORTER_DAEMON_DRAIN_TIMEOUT_MS(60s default, env-var overridable) for the counter to read zero before issuingstop. Pre-v2 daemons are treated as permanently busy, so the drain timeout is also their safety bound.waitForDaemonIdlereturns bothdrainedandreplacedByPeer, and the upgrading client bails out withoutstop.If the daemon is still busy past the timeout the replacement is abandoned and the caller fails loud rather than killing the peer.
Regression coverage
tests/daemon-listtools-progress.test.tsdrives the real client transport against the real host framing over a socket:survives an OAuth wait followed by several paginated tools/list pages— an OAuth wait plus three delayed pages, every phase longer than the socket deadline. Asserts the tools resolve,listToolsran exactly once (no replay), onelistToolsrequest reached the daemon, andlaunchDaemonDetachedwas never called (no restart). Verified it fails without the mechanism: stubbing out the progress emitter makes it die withDaemon did not stop before restart could begin.reaches a deadline shorter than the default progress interval— the P2 case.emits the first progress frame before any cadence interval can elapse— pins the P2 fix at a 30ms deadline, well under the 250ms default cadence.still trips the socket deadline when the daemon stops sending progress frames— silence is still fatal.walks every cursor page but refuses to page forever on a repeated cursor— the P1 case.tests/daemon-client-timeout.test.tspins the budget itself: thelistToolssocket deadline equals the caller deadline, so a phase-count multiplier cannot come back unnoticed.tests/daemon-host.test.tspins the operation ceiling for the fixed-phase methods against a wedged runtime:callTool,listResources, andreadResourceall returnoperation_timeoutafter the sum ofMCPORTER_OAUTH_TIMEOUT_MSandDEFAULT_REQUEST_TIMEOUT_MSEC, using fake timers so the test does not wait the production minute-plus.tests/daemon-client-config-stale.test.tspins the ClawSweeper P1 fix:waits for a busy shared daemon to drain before sending stop— the upgrading client observesactiveRequestsflip to zero before issuingstop, proving no in-flight OAuth or cursor page was killed.does not stop a peer fresh daemon if it replaces the busy one mid-drain— a second client that wins the race to replace the busy daemon during the drain window has its fresh daemon left alone; the upgrading client does not callstopand does not relaunch.refuses to replace a daemon that never drains before the timeout—MCPORTER_DAEMON_DRAIN_TIMEOUT_MS=500exercises the refusal branch without waiting the production minute, asserting the busy peer's daemon is left running.Housekeeping
main.pnpm checkandpnpm testgreen locally (860 passed, 3 skipped); CI green on Ubuntu, macOS and Windows.